Add nonReentrant to relayMessage()
#172
Conversation
There was a problem hiding this comment.
Lookin' good! I see the question above -- to clarify, this will only work in the OVM_L2CrossDomainMessenger. The reason is because of account abstraction, OVM state transitions do not include a from, just a to, meaning that the entrypoint contract will get address(0) for ovmCALLER, during the very first ovmCALL only. Since the contract uses the compiler (+build ovm), we can easily access ovmCALLER with a normal msg.sender.
It strikes me that this property could be worth explicitly adding to our Execution Manager tests which use the ExecutionManagerTestRunner. I think we want to merge this ASAP though, so I'm gonna approve this one and create another ticket for that.
maurelian
force-pushed
the
fix/reentryOnBridge
branch
from
9758ca1
to
5886e76
Compare
contracts/optimistic-ethereum/OVM/bridge/OVM_L1CrossDomainMessenger.sol
Outdated
Show resolved
Hide resolved
| @@ -140,6 +143,7 @@ contract OVM_L1CrossDomainMessenger is iOVM_L1CrossDomainMessenger, OVM_BaseCros | |||
| ) | |||
| override | |||
| public | |||
| nonReentrant | |||
There was a problem hiding this comment.
FWIW, this code path speaks directly to the rollup contracts (OVM_CanonicalTransactionChain.enqueue specifically) so it should not be re-enterable based on the behavior of that target, so it should not be needed.
I'd recommend removing it, because the L1 SSTORE is gonna be expensive. If you think that it's worth keeping, we can, but we should also add it to sendMessage, since the functions serve similar purposes.
There was a problem hiding this comment.
OVM_BaseCrossDomainMessenger.sendMessage() already has the nonReentrant modifier on it. You can see above that I moved it down a couple lines to improve the style. But looking at both sendMessage and replayMessage more closely, I agree that it looks pretty safe to remove them.
contracts/optimistic-ethereum/OVM/bridge/OVM_L2CrossDomainMessenger.sol
Outdated
Show resolved
Hide resolved
Co-authored-by: ben-chain <ben@pseudonym.party>
contracts/optimistic-ethereum/OVM/bridge/OVM_BaseCrossDomainMessenger.sol
Show resolved
Hide resolved
maurelian
changed the title
maurelian
changed the title
relayMessage()
Description
This is a fix to Sam's bug.
Questions
Metadata
Fixes
Contributing Agreement